Skip to content

Set cmake_minimum_required(VERSION 3.22) #175

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maflcko
Copy link
Contributor

@maflcko maflcko commented May 15, 2025

It is unclear if an earlier version is supported, because no one (neither CI nor developers) are testing an earlier version. See also #164.

Also, it is unclear why effort should be made to try to support earlier versions than Bitcoin Core (https://github.com/bitcoin/bitcoin/blob/e486597f9a57903600656fb5106858941885852f/CMakeLists.txt#L10), because libmultiprocess is currently only used there.

Once there is a need or use-case to derive from Bitcoin Core, the policy could be changed then.

Bumping the minimum could also unlock simpler cmake code, see #163 (comment)

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 3f386cc.

@ryanofsky
Copy link
Collaborator

Concept ~0 assuming #163 is merged first.

This PR is doing two different things (see #163 (comment))

  1. It is switching on cmake policies CMP0076-CMP0128
  2. It is triggering a fatal error if versions of cmake before 3.22 are used.

I think the first change is good. The second change seems a like it introduces misleading documentation and an inappropriate error message, but it is not very damaging and seems ok if we want to be paranoid about old versions of cmake doing bad things.

I would like to see #163 merged first because it is only doing the first thing, not the second thing and I do think it is good to keep these things separate for clarity.

@maflcko
Copy link
Contributor Author

maflcko commented May 15, 2025

This PR is doing two different things

correct. Thanks for making it explicit. I like both things and I think it is the most simple and most practical way forward for now.

@DrahtBot
Copy link

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #163 (build: set cmake policy version to 3.31 by purpleKarrot)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants